Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(cfg): graph starts from offset 0x01 instead of 0x00, prune redundant nodes #559

Merged
merged 5 commits into from
Feb 21, 2025
Merged

fix(cfg): graph starts from offset 0x01 instead of 0x00, prune redundant nodes #559

merged 5 commits into from
Feb 21, 2025

Conversation

chen4903
Copy link
Contributor

@chen4903 chen4903 commented Feb 11, 2025 •
edited
Loading

Motivation

Fix this issue: #499

Solution

Subtract 1 from the index, which is operation.last_instruction.instruction, thereby shifting the overall offset to start from 0x00 (currently starting from 0x01).

I believe this approach is appropriate. Because the downstream program does not seem to use this offset, and this offset only serves as the destination for opcodes such as JumpI and related opcodes.

Attention: The full testing has not been completed, so it is unclear whether it will affect other content. (I am not familiar with this repository). Therefore, please identify and test thoroughly.

@chen4903 chen4903 requested a review from Jon-Becker as a code owner February 11, 2025 09:34
@Jon-Becker
Copy link
Owner

thank you for contributing! i will fix the tests soon.

@chen4903 chen4903 changed the title Fix(T-bug): cfg graph starts from offset 0x01 instead of 0x00 Fix(T-bug): (1) cfg graph starts from offset 0x01 instead of 0x00 ; (2) remove redundant nodes Feb 13, 2025
@chen4903
Copy link
Contributor Author

chen4903 commented Feb 13, 2025 •
edited
Loading

As mentioned in #499, there are duplicate nodes in the graph, which leads to incorrect edges. This issue has been fixed by using a mapping to record whether a node has been used: if the node has been used, recursion is not performed.

// (2, 16) is not correct
edges: (0, 1), (0, 2), (2, 3), (3, 4), (4, 5), (5, 6), (5, 7), (7, 8), (7, 9), (9, 10), (9, 11), (4, 12), (3, 13), (13, 14), (13, 15), (2, 16),

// 15 and 16 are redundant
15: "0x31 JUMPDEST \n0x32 PUSH0 0\n0x33 DUP1 \n0x34 REVERT \n",
16: "0x31 JUMPDEST \n0x32 PUSH0 0\n0x33 DUP1 \n0x34 REVERT \n",

And the result of the test is:

Contract Cfg: CfgResult {
    graph: Graph {
        Ty: "Directed",
        node_count: 16,
        edge_count: 15,
        edges: (0, 1), (0, 2), (2, 3), (3, 4), (4, 5), (5, 6), (5, 7), (7, 8), (7, 9), (9, 10), (9, 11), (4, 12), (3, 13), (13, 14), (13, 15),
        node weights: {
            0: "0 PUSH1 0x80\n0x02 PUSH1 0x40\n0x04 MSTORE \n0x05 CALLVALUE \n0x06 DUP1 \n0x07 ISZERO \n0x08 PUSH1 0x0e\n0x0a JUMPI \n",
            1: "0x0b PUSH0 0\n0x0c DUP1 \n0x0d REVERT \n",
            2: "0x0e JUMPDEST \n0x0f POP \n0x10 PUSH1 0x04\n0x12 CALLDATASIZE \n0x13 LT \n0x14 PUSH1 0x30\n0x16 JUMPI \n",
            3: "0x17 PUSH0 0\n0x18 CALLDATALOAD \n0x19 PUSH1 0xe0\n0x1b SHR \n0x1c DUP1 \n0x1d PUSH4 0x2125b65b\n0x22 EQ \n0x23 PUSH1 0x34\n0x25 JUMPI \n",
            4: "0x34 JUMPDEST \n0x35 PUSH1 0x44\n0x37 PUSH1 0x3f\n0x39 CALLDATASIZE \n0x3a PUSH1 0x04\n0x3c PUSH1 0x46\n0x3e JUMP \n0x46 JUMPDEST \n0x47 PUSH0 0\n0x48 DUP1 \n0x49 PUSH0 0\n0x4a PUSH1 0x60\n0x4c DUP5 \n0x4d DUP7 \n0x4e SUB \n0x4f SLT \n0x50 ISZERO \n0x51 PUSH1 0x57\n0x53 JUMPI \n",
            5: "0x57 JUMPDEST \n0x58 DUP4 \n0x59 CALLDATALOAD \n0x5a PUSH4 0xffffffff\n0x5f DUP2 \n0x60 AND \n0x61 DUP2 \n0x62 EQ \n0x63 PUSH1 0x69\n0x65 JUMPI \n",
            6: "0x66 PUSH0 0\n0x67 DUP1 \n0x68 REVERT \n",
            7: "0x69 JUMPDEST \n0x6a SWAP3 \n0x6b POP \n0x6c PUSH1 0x20\n0x6e DUP5 \n0x6f ADD \n0x70 CALLDATALOAD \n0x71 PUSH1 0x01\n0x73 PUSH1 0x01\n0x75 PUSH1 0xa0\n0x77 SHL \n0x78 SUB \n0x79 DUP2 \n0x7a AND \n0x7b DUP2 \n0x7c EQ \n0x7d PUSH1 0x83\n0x7f JUMPI \n",
            8: "0x80 PUSH0 0\n0x81 DUP1 \n0x82 REVERT \n",
            9: "0x83 JUMPDEST \n0x84 SWAP2 \n0x85 POP \n0x86 PUSH1 0x40\n0x88 DUP5 \n0x89 ADD \n0x8a CALLDATALOAD \n0x8b PUSH1 0x01\n0x8d PUSH1 0x01\n0x8f PUSH1 0xe0\n0x91 SHL \n0x92 SUB \n0x93 DUP2 \n0x94 AND \n0x95 DUP2 \n0x96 EQ \n0x97 PUSH1 0x9d\n0x99 JUMPI \n",
            10: "0x9a PUSH0 0\n0x9b DUP1 \n0x9c REVERT \n",
            11: "0x9d JUMPDEST \n0x9e DUP1 \n0x9f SWAP2 \n0xa0 POP \n0xa1 POP \n0xa2 SWAP3 \n0xa3 POP \n0xa4 SWAP3 \n0xa5 POP \n0xa6 SWAP3 \n0xa7 JUMP \n0x3f JUMPDEST \n0x40 POP \n0x41 POP \n0x42 POP \n0x43 JUMP \n0x44 JUMPDEST \n0x45 STOP \n",
            12: "0x54 PUSH0 0\n0x55 DUP1 \n0x56 REVERT \n",
            13: "0x26 DUP1 \n0x27 PUSH4 0xb69ef8a8\n0x2c EQ \n0x2d PUSH1 0x44\n0x2f JUMPI \n",
            14: "0x44 JUMPDEST \n0x45 STOP \n",
            15: "0x30 JUMPDEST \n0x31 PUSH0 0\n0x32 DUP1 \n0x33 REVERT \n",
        },
        edge weights: {
            0: "false",
            1: "true",
            2: "false",
            3: "true",
            4: "true",
            5: "false",
            6: "true",
            7: "false",
            8: "true",
            9: "false",
            10: "true",
            11: "false",
            12: "false",
            13: "true",
            14: "true",
        },
    },
}

@chen4903
Copy link
Contributor Author

hi @Jon-Becker , I think the change is ready. Thanks for review.

@Jon-Becker
Copy link
Owner

i will review tonight 🫡

Jon-Becker
Jon-Becker requested changes Feb 19, 2025
View reviewed changes
Copy link
Owner

@Jon-Becker Jon-Becker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! just one nit comment. I'll fix the other CFG tests tonight and we can get this merged!

thank you for contributing 

@Jon-Becker Jon-Becker changed the title Fix(T-bug): (1) cfg graph starts from offset 0x01 instead of 0x00 ; (2) remove redundant nodes fix(cfg): graph starts from offset 0x01 instead of 0x00, prune redundant nodes Feb 21, 2025
@Jon-Becker Jon-Becker changed the title fix(cfg): graph starts from offset 0x01 instead of 0x00, prune redundant nodes fix(cfg): graph starts from offset 0x01 instead of 0x00, prune redundant nodes Feb 21, 2025
@Jon-Becker
Copy link
Owner

tests passing locally, env rpc issues here so they wont on these runners. merging anyway :)

@Jon-Becker Jon-Becker merged commit 55ee688 into Jon-Becker:main Feb 21, 2025
@chen4903 chen4903 mentioned this pull request Feb 26, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jon-Becker Jon-Becker Jon-Becker approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chen4903 @Jon-Becker